Fail the meta-test when a 500 response is returned

In AuthenticatedEndpoints_NeverReturn500WithStackTrace, the test only inspects the body if status is 500, but never asserts that 500 is forbidden, so a plain 500 response without stack-trace text still passes. That creates a false negative for the exact regression this meta-test is supposed to catch (NeverReturn500), allowing server errors on these endpoints to slip through CI unnoticed.

Useful? React with 👍 / 👎.

Assert success for valid webhook URLs

CreateWebhook_WithValidUrl_Succeeds currently checks only status < 500, which means a regression that rejects all valid HTTPS webhook URLs with 4xx would still pass this test. Because this method is the only positive-path check in this file, it should assert a success/acceptance status to actually protect valid URL behavior.

Useful? React with 👍 / 👎.

Remove hard wall-clock threshold from unit test

The test enforces elapsed < 1000ms for JSON serialize/parse of a 50K string, which depends on CI machine load and runtime variance rather than functional correctness. On slower or contended runners this can fail intermittently even when behavior is correct, introducing flaky test failures into the suite.

Useful? React with 👍 / 👎.

The System.Random class is not thread-safe. Since this Rng instance is static and shared across the class, concurrent execution of tests (which can occur in xUnit depending on configuration) may lead to race conditions, resulting in corrupted state or zero values. Consider using lock (Rng) when accessing it, or using a thread-safe alternative if parallelism is expected.

Adversarial tests should ideally include null bytes (\0), as they are a common source of vulnerabilities and crashes in string handling, database drivers, and serialization logic. Since the PR summary mentions exercising null bytes, they should be included in this randomized sweep as well.

                0 => (char)Rng.Next(0, 32),               // control chars (including null)

The method RandomGuidString is currently unused in this test file. To maintain code cleanliness, it should be removed if there are no immediate plans to use it.

Using DateTimeOffset.UtcNow inside an FsCheck generator makes the property test non-deterministic. If a failure occurs, the reproduction of that specific failure using the same seed might fail because the timestamp will have changed. It is better to generate the DateTimeOffset using FsCheck's built-in generators to ensure full reproducibility.

The use of Thread.Sleep(1) inside a property-based test (which runs 200 iterations by default) significantly slows down the test suite. Furthermore, DateTimeOffset.UtcNow resolution on some operating systems can be as high as 15ms, so a 1ms sleep does not guarantee that the timestamp will advance. Since the assertion on line 385 uses BeOnOrAfter, the test will pass even if the timestamps are identical. Removing the sleeps will improve performance without affecting the validity of the current assertion.

                switch (entityType)
                {
                    case "board":
                        var board = new Board("Test");
                        initialUpdatedAt = board.UpdatedAt;
                        board.Update(name: "Updated");
                        afterUpdatedAt = board.UpdatedAt;
                        break;
                    case "card":
                        var card = new Card(Guid.NewGuid(), Guid.NewGuid(), "Title");
                        initialUpdatedAt = card.UpdatedAt;
                        card.Update(description: "Updated");
                        afterUpdatedAt = card.UpdatedAt;
                        break;
                    case "column":
                        var col = new Column(Guid.NewGuid(), "Col", 0);
                        initialUpdatedAt = col.UpdatedAt;
                        col.SetPosition(1);
                        afterUpdatedAt = col.UpdatedAt;
                        break;
                    case "label":
                        var label = new Label(Guid.NewGuid(), "Label", "#FF0000");
                        initialUpdatedAt = label.UpdatedAt;
                        label.Update(name: "Updated");
                        afterUpdatedAt = label.UpdatedAt;
                        break;
                    default:
                        throw new InvalidOperationException($"Unknown entity type: {entityType}");
                }

Require UpdatedAt to move forward after entity updates

This property claims to verify that updates advance UpdatedAt, but afterUpdatedAt.Should().BeOnOrAfter(initialUpdatedAt) also passes when timestamps are equal. If a regression stops touching UpdatedAt in Board.Update/Card.Update/Column.SetPosition/Label.Update, this test still passes with afterUpdatedAt == initialUpdatedAt, so it cannot catch the intended failure mode.

Useful? React with 👍 / 👎.

Remove swallowed parse errors from malformed-params test

The assertion is ineffective because JSON.parse(param) is wrapped in an inner try/catch that suppresses all errors before expect(...).not.toThrow() runs. That means this test passes regardless of malformed input behavior and cannot detect regressions in how malformed operation parameters are handled.

Useful? React with 👍 / 👎.

This test is effectively a no-op: JSON.parse errors are caught and swallowed inside the expectation, so the outer expect().not.toThrow() will always pass regardless of input. Consider exercising the actual parsing/validation helper the app uses (or add one) and assert a concrete outcome (e.g., returns null/Result/error state) for malformed parameter strings.

Same issue as above: the try/catch inside expect().not.toThrow() swallows any JSON.stringify/JSON.parse failures, so this assertion will always pass and doesn't validate behavior. If the goal is resilience, assert that serialization succeeds for the supported patterns, and explicitly assert failure/handling behavior for patterns you expect to be rejected.

This uses a hard 1s wall-clock threshold (performance.now()) which is prone to CI flakiness under load and doesn't really measure UI “freeze” (it only measures JSON stringify/parse). Consider removing the timing assertion (just ensure it completes), or make it much more tolerant and/or run it as a benchmark-style test outside the main suite.

This test doesn’t actually verify that a due date can be cleared: the card starts with DueDate == null, and Card.Update(dueDate: null) is a no-op with the current Card.Update implementation (it only assigns when dueDate.HasValue). To test “accepts null/clear”, first set a non-null due date, then call Update(dueDate: null) and assert it becomes null (or rename the test if clearing isn’t supported).

The stack-trace body check is unreachable: the BeLessThan(500) assertion will already fail the test before the subsequent if (StatusCode >= 500) block can run. If you want to assert both “not 500” and “no stack trace if 500”, capture the body first and/or use an AssertionScope so both checks can be evaluated.

Same unreachable-code issue here: the BeLessThan(500) assertion will throw before the if (StatusCode >= 500) branch can execute, so the response-body redaction check never runs. Consider restructuring to always read/assert on the body when status >= 500 (or use AssertionScope).

Rng is a shared static Random consumed across multiple [Fact]s. Because xUnit does not guarantee test execution order, the sequence of “random” values (and therefore failures) can change between runs despite the fixed seed. To keep runs reproducible, instantiate a new Random(42) per test (or reset the RNG at the start of each test), rather than sharing a single static instance.

The XML doc says adversarial URLs “are rejected with 4xx — never 5xx”, but the assertion only checks < 500, so 2xx responses (accepting a dangerous URL) would still pass. If the requirement is rejection, assert a 4xx response (and consider environment-specific exceptions like localhost being allowed in Development per OutboundWebhooksApiTests.cs:104-113).

Test name says “Succeeds”, but the assertion only checks that the response is not a 5xx. Either tighten the assertion to the expected success status (e.g., 201 Created) or rename the test to reflect the weaker contract (“never 500”).

Assert 4xx rejection for adversarial webhook endpoints

CreateWebhook_WithAdversarialUrl_NeverReturns500 currently only asserts status < 500, so a regression that starts accepting dangerous endpoints (for example javascript:/data:/metadata-host URLs) with 2xx would still pass this test. That makes this adversarial security test a false negative for URL-validation/SSRF regressions; it should assert explicit rejection semantics (4xx), with any intentionally allowed cases split into separate tests.

Useful? React with 👍 / 👎.